Copy node_modules to platform on prepare (fix Node6/npm 3.x bug) #2152

petekanev · 2016-10-21T13:30:54Z

Changes in this PR will allow copying of all production (non-dev) node_modules into the platform's tns_modules independent of whether they have been installed using tns install (npm 2.x) or npm install with npm version 3+.

The proposed implementation however will iterate through the production modules' package.jsons on every build. Traversal of all package's production dependencies is also a bit slower than before. I'll look into a way to address only changes on subsequent builds, and potentially optimize traversals.

Issue #2149 and the respective PR will most likely address preparing of modules on each rebuild. Has yet to be verified.

There are currently 3 failing tests testing how and where npm_modules are copied into the platforms dir.

The changes have been tested with the Groceries app (master branch) with both tns install (npm 2), and npm install (npm 3)

Addresses issues: #1989, #1845, #1740

Also invalidates PR #2136

Depends on changes in both runtimes: NativeScript/android#587, NativeScript/ios-jsc#673;

ping: @rosen-vladimirov @hdeshev @Plamen5kov @atanasovg

copy production node_modules as-is to the project's tns_modules

hdeshev

I have two concerns with this PR:

The getProductionDependencies method is too complex and needs to be broken up in small chunks, cleaned up, and possibly moved to a different class.
I do not see anything preventing pulling two versions of a package (think huge packages like Angular) to your app's tns_modules dir, if it has been requested by your app's dependencies. What should we do in that case?

hdeshev · 2016-10-24T08:14:05Z

lib/tools/node-modules/node-modules-builder.ts

+			// console.log(productionDependencies);
+
+			// TODO: Pip3r4o - result is not used currently
+			// let nodeModules = this.getChangedNodeModules(absoluteOutputPath, platform, lastModifiedTime).wait();


Can we get rid of commented out code?

hdeshev · 2016-10-24T08:14:43Z

lib/tools/node-modules/node-modules-builder.ts

+		var deps: any = [];
+		var seen: any = {};
+
+		var pJson = path.join(projectPath, "package.json");


Please, avoid weird abbreviations like pJson. How about packageJsonPath?

hdeshev · 2016-10-24T08:15:23Z

lib/tools/node-modules/node-modules-builder.ts

+
+			}
+			// module is scoped 
+			else {


hdeshev · 2016-10-24T08:15:37Z

lib/tools/node-modules/node-modules-builder.ts

+					var dep = addDependency(deps, name, scopedModulePath, 0);
+					traverseModule(scopedModulePath, depth, dep);
+				}
+				else {


hdeshev · 2016-10-24T08:17:24Z

lib/tools/node-modules/node-modules-builder.ts

+						return;
+					}
+
+					var dep = addDependency(deps, name, modulePath, 0);


Again, please avoid abbreviations: dep

hdeshev · 2016-10-24T08:30:06Z

lib/tools/node-modules/node-modules-builder.ts

+
+		return filterUniqueDirectories(deps);
+
+		function traverseChild(name: string, currentModulePath: string, depth: number) {


Those inner functions are very confusing and unreadable. Consider moving them to methods of this class or an entirely new class.

hdeshev · 2016-10-24T08:33:50Z

lib/tools/node-modules/node-modules-builder.ts

+				}
+			}
+
+			function traverseModule(modulePath: string, depth: number, currentDependency: any) {


What is the difference when traversing a "child" and a "module". I think one of these functions needs a better name.

Also please add return type to the function

hdeshev · 2016-10-24T08:38:03Z

lib/tools/node-modules/node-modules-builder.ts

+			}
+		}
+
+		function filterUniqueDirectories(dependencies: any) {


Can we use a map keyed by the dependency directory here (simulating a set collection) instead of doing linear traversals all the time?

Noted. I used a string map initially to check if the plugin's already present in the list, but that approach omitted corner cases where 2 plugins with different versions were present. I'll revisit it again

petekanev · 2016-10-24T08:55:53Z

@hdeshev about 2. - currently we will pull both versions of the package, as extracted by npm, you can't tell by version alone if any of the packages' dependents are compatible with all, or just 1 of said package's version.
Previously only the package with the greatest version will be pulled from node_modules, which is not always desired.

rosen-vladimirov · 2016-10-24T10:12:57Z

lib/tools/node-modules/node-modules-builder.ts

+		let pJson = path.join(projectPath, "package.json");
+		let nodeModulesDir = path.join(projectPath, "node_modules");
+
+		let content = require(pJson);


I would advise using $fs as this will make the code more testable:

this.$fs.readJson(pJson).wait();

rosen-vladimirov · 2016-10-24T10:35:01Z

lib/tools/node-modules/node-modules-builder.ts

+
+		let content = require(pJson);
+
+		Object.keys(content.dependencies).forEach((key) => {


if there's no dependencies section in the package.json, this code will fail. I would use lodash's _.each:

_.keys(content.dependencies) .forEach(key => { .... });

rosen-vladimirov · 2016-10-24T10:40:21Z

lib/tools/node-modules/node-modules-builder.ts

+					}
+
+					if (packageJsonContents.dependencies) {
+						Object.keys(packageJsonContents.dependencies).forEach((key) => {


Same as above - it's safer to use:

_.keys(packageJsonContent.dependencies).forEach(...

rosen-vladimirov · 2016-10-24T10:41:56Z

lib/tools/node-modules/node-modules-builder.ts

+				}
+			}
+
+			function addDependency(deps: any[], name: string, directory: string, depth: number) {


this function needs return type

rosen-vladimirov · 2016-10-24T10:42:16Z

lib/tools/node-modules/node-modules-builder.ts

+				}
+			}
+
+			function traverseModule(modulePath: string, depth: number, currentDependency: any) {


Also please add return type to the function

rosen-vladimirov · 2016-10-24T10:44:12Z

lib/tools/node-modules/node-modules-builder.ts

+			}
+
+			function addDependency(deps: any[], name: string, directory: string, depth: number) {
+				let dep: any = {};


you can use shorter syntax here:

let dep = { name, directory, depth };

rosen-vladimirov · 2016-10-24T10:48:16Z

lib/tools/node-modules/node-modules-builder.ts

+		function filterUniqueDirectories(dependencies: any) {
+			let unique: any = [];
+			let distinct: any = [];
+			for (let i in dependencies) {


You can get directly the dependency by using:

for (let dep of dependencies) { if (distinct.indexOf(dep.directory) > -1) { .... }

rosen-vladimirov · 2016-10-24T10:48:34Z

lib/tools/node-modules/node-modules-dest-copy.ts

-	public copyModules(dependencies: IDictionary<ILocalDependencyData>, platform: string): void {
-		_.each(dependencies, dependency => {
+	public copyModules(dependencies: any[], platform: string): void {
+		for (var entry in dependencies) {


rosen-vladimirov · 2016-10-24T10:48:41Z

lib/tools/node-modules/node-modules-dest-copy.ts

-		_.each(dependencies, dependency => {
+	public copyModules(dependencies: any[], platform: string): void {
+		for (var entry in dependencies) {
+			var dependency = dependencies[entry];


rosen-vladimirov · 2016-10-24T10:49:56Z

lib/tools/node-modules/node-modules-dest-copy.ts

+
+			shelljs.mkdir("-p", targetDir);
+			shelljs.cp("-Rf", dependency.directory, targetDir);
+			// console.log("Copied dependency: " + dependency.name);


can you remove the commented line or add it as log trace message:

this.$logger.trace(`Copied dependency: ${dependency.name}`);

petekanev · 2016-10-24T11:57:43Z

@hdeshev @rosen-vladimirov addressed concerns with style and readability

hdeshev · 2016-10-24T12:12:48Z

lib/tools/node-modules/node-modules-dependencies-builder.ts

+        return dependency;
+    }
+
+    private ensureModuleExists(modulePath: string): boolean {


I'd rename this to moduleExists. Nothing to ensure anyway.

hdeshev · 2016-10-24T12:17:22Z

lib/tools/node-modules/node-modules-dependencies-builder.ts

+        // check if key appears in a scoped module dependency
+        let isScoped = name.indexOf('@') === 0;
+
+        if (!isScoped) {


The scoped/unscoped branches seem to be mostly the same code copied/pasted over. Can we eliminate the duplication in a shared method and pass the different paths as parameters?

You're right! I seem to have missed it!

fix broken tests

…ns_modules (npm2 hacky fix)

Plamen5kov · 2016-10-26T07:43:31Z

👍 looks good, the sooner this is in master, the better

A while ago, we had a logic to find out which node_modules should be processed. However it is not used anymore (removed in #2152 ). So remove the code - we do not need it anymore.

* Fix Android debugging When executing `tns debug android` for slow emulators/devices we have logic to wait several seconds until debugger is attached. However we've missed to `await` the `sleep` method and in fact we are not waiting at all. Add the missing await, which fixes the `tns debug android` command. * Remove unused code A while ago, we had a logic to find out which node_modules should be processed. However it is not used anymore (removed in #2152 ). So remove the code - we do not need it anymore. * Fix type

run npm list --prod command and get all dependency names

fdec58f

petekanev added this to the 2.4.0 milestone Oct 21, 2016

add npm-independent traversal to get project's production dependencies

77f86a3

copy production node_modules as-is to the project's tns_modules

petekanev force-pushed the pete/copy-node_modules branch from 9397e32 to 77f86a3 Compare October 21, 2016 14:01

petekanev added the 2 - Ready For Test label Oct 21, 2016

petekanev force-pushed the pete/copy-node_modules branch from 6b8c084 to ec9f70a Compare October 24, 2016 08:40

hdeshev suggested changes Oct 24, 2016

View reviewed changes

rosen-vladimirov suggested changes Oct 24, 2016

View reviewed changes

petekanev force-pushed the pete/copy-node_modules branch 2 times, most recently from b66b4a7 to cbfd5b8 Compare October 24, 2016 11:56

hdeshev suggested changes Oct 24, 2016

View reviewed changes

petekanev force-pushed the pete/copy-node_modules branch 2 times, most recently from f4a99cd to 1b80824 Compare October 25, 2016 06:44

separate logic for determining production dependencies in a class

7b10417

fix broken tests

petekanev force-pushed the pete/copy-node_modules branch from 1b80824 to 7b10417 Compare October 25, 2016 08:54

Plamen5kov mentioned this pull request Oct 25, 2016

don't use flatten logic #2136

Closed

Plamen5kov and others added 2 commits October 25, 2016 15:17

removed unnecessary dependency

4c483d0

remove node_modules of tns-core-modules if present when copied into t…

b569b6e

…ns_modules (npm2 hacky fix)

petekanev merged commit 0bc09ec into master Oct 26, 2016

petekanev mentioned this pull request Oct 26, 2016

Extract tns-core-modules on root level of tns_modules #2156

Merged

hdeshev mentioned this pull request Nov 4, 2016

Keep track of prepared plugins and avoid unnecessary cleans. #2194

Merged

ivanbuhov mentioned this pull request Nov 21, 2016

CLI fails when preparing plugin with symlink on Mac #2242

Closed

rosen-vladimirov added a commit that referenced this pull request Apr 13, 2017

Remove unused code

bf3f508

A while ago, we had a logic to find out which node_modules should be processed. However it is not used anymore (removed in #2152 ). So remove the code - we do not need it anymore.

rosen-vladimirov mentioned this pull request Apr 13, 2017

Fix debug android command #2713

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy node_modules to platform on prepare (fix Node6/npm 3.x bug) #2152

Copy node_modules to platform on prepare (fix Node6/npm 3.x bug) #2152

petekanev commented Oct 21, 2016 •

edited

Loading

hdeshev left a comment

hdeshev Oct 24, 2016

hdeshev Oct 24, 2016

hdeshev Oct 24, 2016

hdeshev Oct 24, 2016

hdeshev Oct 24, 2016

hdeshev Oct 24, 2016

hdeshev Oct 24, 2016

rosen-vladimirov Oct 24, 2016

hdeshev Oct 24, 2016

petekanev Oct 24, 2016

petekanev commented Oct 24, 2016

rosen-vladimirov Oct 24, 2016

rosen-vladimirov Oct 24, 2016

rosen-vladimirov Oct 24, 2016

rosen-vladimirov Oct 24, 2016

rosen-vladimirov Oct 24, 2016

rosen-vladimirov Oct 24, 2016

rosen-vladimirov Oct 24, 2016

rosen-vladimirov Oct 24, 2016

rosen-vladimirov Oct 24, 2016

rosen-vladimirov Oct 24, 2016

petekanev commented Oct 24, 2016

hdeshev Oct 24, 2016

hdeshev Oct 24, 2016

petekanev Oct 24, 2016

Plamen5kov commented Oct 26, 2016


		return filterUniqueDirectories(deps);

		function traverseChild(name: string, currentModulePath: string, depth: number) {


		let content = require(pJson);

		Object.keys(content.dependencies).forEach((key) => {

Copy node_modules to platform on prepare (fix Node6/npm 3.x bug) #2152

Copy node_modules to platform on prepare (fix Node6/npm 3.x bug) #2152

Conversation

petekanev commented Oct 21, 2016 • edited Loading

hdeshev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petekanev commented Oct 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petekanev commented Oct 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Plamen5kov commented Oct 26, 2016

petekanev commented Oct 21, 2016 •

edited

Loading